8343789: Move mutable nmethod data out of CodeCache #21276

bulasevich · 2024-10-01T02:10:37Z

Statistics for 21032 bytecoded nmethods for C1:
 total size      = 120722408 (100%)
 in CodeCache    = 79358808 (65.736603%)
   header        = 5215936 (6.572599%)
   constants     = 320 (0.000403%)
   main code     = 69017912 (86.969444%)
   stub code     = 5124640 (6.457557%)
 mutable data    = 10488856 (8.688409%)
   relocation    = 6573064 (62.667118%)
   oops          = 515680 (4.916456%)
   metadata      = 3400112 (32.416424%)
 immutable data  = 30874744 (25.574991%)
   dependencies  = 636240 (2.060714%)
   nul chk table = 756920 (2.451583%)
   handler table = 180456 (0.584478%)
   scopes pcs    = 16052608 (51.992683%)
   scopes data   = 13248520 (42.910542%)
Statistics for 8171 bytecoded nmethods for C2:
 total size      = 64948664 (100%)
 in CodeCache    = 25580504 (39.385727%)
   header        = 2026408 (7.921689%)
   constants     = 448 (0.001751%)
   main code     = 20925472 (81.802422%)
   stub code     = 2628176 (10.274137%)
 mutable data    = 6572064 (10.118859%)
   relocation    = 3406216 (51.828709%)
   oops          = 305912 (4.654733%)
   metadata      = 2859936 (43.516556%)
 immutable data  = 32796096 (50.495411%)
   dependencies  = 926992 (2.826532%)
   nul chk table = 537024 (1.637463%)
   handler table = 1695568 (5.170030%)
   scopes pcs    = 15451968 (47.115265%)
   scopes data   = 14184544 (43.250710%)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8343789: Move mutable nmethod data out of CodeCache (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21276/head:pull/21276
$ git checkout pull/21276

Update a local copy of the PR:
$ git checkout pull/21276
$ git pull https://git.openjdk.org/jdk.git pull/21276/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21276

View PR using the GUI difftool:
$ git pr show -t 21276

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21276.diff

bridgekeeper · 2024-10-01T02:11:17Z

👋 Welcome back bulasevich! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-01T02:11:29Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-10-01T02:12:04Z

@bulasevich The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

bulasevich · 2024-10-03T15:26:12Z

@vnkozlov Hi Vladimir,
What do you think about the idea of moving relocInfo data out of nmethod additionally to recent Move immutable nmethod data from CodeCache? It would reduce the CodeHeap fill by 5%.

vnkozlov

First, thank you for doing his work.

Main question is: why you did it only for nmethod?

Second question: do you see any performance effects with this change?
My concern is that we iterate relocation info data from different memory space to patch code.

vnkozlov · 2024-11-07T22:59:07Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

-    ldr_constant(dst, Address(dummy, rspec));
+    mov(dst, Address(dummy, rspec));


Why this is needed?

it is not a load from a Constant Pool, so calling ldr_constant here is seems incorrect

the ldr_constant function utilizes either ldr (with a range limit of ±1MB) or, when -XX:-NearCpool is enabled, adrp (range limit of ±2GB) followed by ldr — both of which may fall short when mutable data is allocated on the C heap.

vnkozlov · 2024-11-07T23:02:49Z

src/hotspot/share/code/codeBlob.cpp

  return size;
 }

 CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, CodeBuffer* cb, int size, uint16_t header_size,
-                   int16_t frame_complete_offset, int frame_size, OopMapSet* oop_maps, bool caller_must_gc_arguments) :
+                   int16_t frame_complete_offset, int frame_size, OopMapSet* oop_maps, bool caller_must_gc_arguments,
+                   bool external_mutable_data) :


I suggest to add assert(!external_mutable_data || (kind == CodeBlobKind::Nmethod)

vnkozlov · 2024-11-07T23:07:15Z

src/hotspot/share/code/codeBlob.hpp

+  address _mutable_data;
+  int     _mutable_data_size;
+


Should we add special CodeBlob subclass for nmethod to avoid increase of size for all blobs and stubs?

vnkozlov · 2024-11-07T23:09:08Z

src/hotspot/share/runtime/vmStructs.cpp

+  nonstatic_field(nmethod,                     _mutable_data,                                 address)                               \
+  nonstatic_field(nmethod,                     _mutable_data_size,                            int)                                   \


They are filed in CodeBlob and not in nmethod.

vnkozlov · 2024-11-07T23:46:25Z

Note, with https://bugs.openjdk.org/browse/JDK-8334691 and other changes I moving into direction to make relocation info data immutable. It is already "immutable" in mainline JDK after https://bugs.openjdk.org/browse/JDK-8333819. But it is still mutable in Leyden because we have to patch indexes during publishing nmethod.

My idea was to move relocation info data (which has big size) into immutable data section of nmethod. And leave mutable _oops and _metadata together with code since they are relatively small and we need to patch them together with code.

vnkozlov · 2024-11-07T23:47:50Z

Mutable sizes % do not add up:

mutable data    = 6071648 (9.396180%)
   relocation    = 3437176 (12.846409%)
   oops          = 239488 (0.895084%)
   metadata      = 2394984 (8.951227%)

bulasevich · 2024-11-09T11:37:43Z

Main question is: why you did it only for nmethod?

Yes. I did it symmetrically to a separate immutable data storage for nmethod. Now I see I do not like implementation where for some blobs relocation info is local, but for other it stays aside. I am going to rework that.

any performance effects with this change?

On the aarch machine I see a slight improvement on the big benchmark caused by the code sparsity improvement. Though I need to do more benchmarking to make sure I am not making things worse for others.

Benchmark              Mode  Cnt    Score   Error  Units  |  Benchmark              Mode  Cnt    Score   Error  Units
JmhDotty.runOperation    ss  999  861.717 ± 1.543  ms/op  |  JmhDotty.runOperation    ss  999  840.959 ± 1.473  ms/op
                                                          |
       34555411781      cache-misses:u                    |         34343012187      cache-misses:u
     2913869717708      cpu-cycles:u                      |       2863838151745      cpu-cycles:u
     4185324759051      instructions:u                    |       4209616523046      instructions:u            
     1460914744576      L1-icache-loads:u                 |       1452066316397      L1-icache-loads:u
       97806845375      L1-icache-load-misses:u           |         93815390496      L1-icache-load-misses:u   
     1191854820746      iTLB-loads:u                      |       1169231847276      iTLB-loads:u
       10591067761      iTLB-load-misses:u                |         10134696419      iTLB-load-misses:u        
      838964735227      branch-loads:u                    |        838353168582      branch-loads:u
       25829615231      branch-load-misses:u              |         24361474411      branch-load-misses:u
      836291984964      br_pred:u                         |        838153583659      br_pred:u
       25733552818      br_mis_pred:u                     |         24353396612      br_mis_pred:u
         562168308      group0-code_sparsity:u            |           449848707      group0-code_sparsity:u

bulasevich · 2024-11-09T11:42:02Z

Mutable sizes % do not add up:

Thanks. The correct sizes:

Statistics for 21032 bytecoded nmethods for C1:
 mutable data    = 10488856 (8.688409%)
   relocation    = 6573064 (62.667118%)
   oops          = 515680 (4.916456%)
   metadata      = 3400112 (32.416424%)
Statistics for 8171 bytecoded nmethods for C2:
 mutable data    = 6572064 (10.118859%)
   relocation    = 3406216 (51.828709%)
   oops          = 305912 (4.654733%)
   metadata      = 2859936 (43.516556%)

bulasevich · 2024-11-09T11:47:58Z

My idea was to move relocation info data (which has big size) into immutable data section of nmethod. And leave mutable _oops and _metadata together with code since they are relatively small and we need to patch them together with code.

Hmm. If relocation info goes to an immutable blob, oops+metadata hardly deserves a separate blob.

bulasevich · 2024-11-14T13:56:50Z

Performance update. On an aarch machine the CodeCacheStress benchmark shows a 1-2% performance improvement with this change,

Statistics on the CodeCacheStress benchmark with high numberOfClasses-instanceCount-rangeOfClasses parameter values:

nmethod_count:134000, total_compilation_time: 510460ms
total allocation time malloc_mutable/malloc_immutable/CodeCache_alloc: 62ms/114ms/6333ms,
total allocation size (mutable/immutable/nmentod): 64MB/192MB/488MB

openjdk bot added the hotspot-compiler [email protected] label Oct 1, 2024

bulasevich force-pushed the mutable_data branch from b15f504 to 531b630 Compare October 24, 2024 13:27

bulasevich force-pushed the mutable_data branch 2 times, most recently from 9444108 to c90f5b7 Compare November 7, 2024 19:03

bulasevich changed the title ~~move nmethods relocInfo data to C heap~~ 8343789: Move mutable nmethod data out of CodeCache Nov 7, 2024

vnkozlov reviewed Nov 7, 2024

View reviewed changes

bulasevich added 2 commits November 9, 2024 14:51

8343789: Move mutable nmethod data out of CodeCache

d72804e

fix stat printout

52dd527

bulasevich force-pushed the mutable_data branch from c90f5b7 to 52dd527 Compare November 9, 2024 12:01

bulasevich force-pushed the mutable_data branch from 31dffae to d97dc92 Compare November 15, 2024 18:06

update: mutable data for all CodeBlobs with relocations

c401e77

bulasevich force-pushed the mutable_data branch from d97dc92 to c401e77 Compare November 15, 2024 22:14

bulasevich added 2 commits November 19, 2024 00:52

update jvm.hotspot.code.CodeBlob class

0c70bd2

remove _code_end_offset

88e3106

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343789: Move mutable nmethod data out of CodeCache #21276

8343789: Move mutable nmethod data out of CodeCache #21276

bulasevich commented Oct 1, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

bulasevich commented Oct 3, 2024

vnkozlov left a comment

vnkozlov Nov 7, 2024

bulasevich Nov 9, 2024

vnkozlov Nov 7, 2024

vnkozlov Nov 7, 2024

vnkozlov Nov 7, 2024

vnkozlov commented Nov 7, 2024

vnkozlov commented Nov 7, 2024

bulasevich commented Nov 9, 2024 •

edited

Loading

bulasevich commented Nov 9, 2024 •

edited

Loading

bulasevich commented Nov 9, 2024

bulasevich commented Nov 14, 2024

		ldr_constant(dst, Address(dummy, rspec));
		mov(dst, Address(dummy, rspec));

		nonstatic_field(nmethod, _mutable_data, address) \
		nonstatic_field(nmethod, _mutable_data_size, int) \

8343789: Move mutable nmethod data out of CodeCache #21276

Are you sure you want to change the base?

8343789: Move mutable nmethod data out of CodeCache #21276

Conversation

bulasevich commented Oct 1, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

bulasevich commented Oct 3, 2024

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Nov 7, 2024

Choose a reason for hiding this comment

bulasevich Nov 9, 2024

Choose a reason for hiding this comment

vnkozlov Nov 7, 2024

Choose a reason for hiding this comment

vnkozlov Nov 7, 2024

Choose a reason for hiding this comment

vnkozlov Nov 7, 2024

Choose a reason for hiding this comment

vnkozlov commented Nov 7, 2024

vnkozlov commented Nov 7, 2024

bulasevich commented Nov 9, 2024 • edited Loading

bulasevich commented Nov 9, 2024 • edited Loading

bulasevich commented Nov 9, 2024

bulasevich commented Nov 14, 2024

bulasevich commented Oct 1, 2024 •

edited by openjdk bot

Loading

bulasevich commented Nov 9, 2024 •

edited

Loading

bulasevich commented Nov 9, 2024 •

edited

Loading